Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always use G1GC for product tests #21311

Merged
merged 1 commit into from
Apr 30, 2024
Merged

Always use G1GC for product tests #21311

merged 1 commit into from
Apr 30, 2024

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Mar 28, 2024

No description provided.

@cla-bot cla-bot bot added the cla-signed label Mar 28, 2024
@wendigo wendigo requested a review from findepi March 28, 2024 15:12
@wendigo
Copy link
Contributor Author

wendigo commented Mar 28, 2024

@findepi do you recall why we've used other GC settings for some of the product test environments?

@findepi
Copy link
Member

findepi commented Mar 28, 2024

why we've used other GC settings for some of the product test environments?

yes, to reduce memory footprint.

production deployments start with 64GB main memory, and product tests run in a very constraint environment.
Most of them should take less than 4 GB (for all services), and G1 isn't good for such cases.

@wendigo
Copy link
Contributor Author

wendigo commented Mar 28, 2024

@findepi since then github workers doubled the memory. It seems that this change builds fine so I'd propose to merge it and observe the stability of the CI and revert if it degrades. WDYT?

@wendigo
Copy link
Contributor Author

wendigo commented Mar 28, 2024

@findepi
Copy link
Member

findepi commented Mar 28, 2024

i don't see a point in such a change, nor i want to use more when running locally.

@wendigo
Copy link
Contributor Author

wendigo commented Mar 28, 2024

@findepi consistency. We only used non-G1 for some product test environments but not for others. G1 is recommended and used in production environments. If there will be some problem with next JDK version and G1 I want to learn from the CI, not from production environments. Hence keeping it consistent with our own recommendations is desirable from my perspective.

@wendigo wendigo requested review from electrum and martint March 28, 2024 17:37
Copy link

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Apr 19, 2024
@mosabua
Copy link
Member

mosabua commented Apr 23, 2024

I agree with the idea that we should use consistent JVM setting across docs, tests, and everything else. Or alternatively if different JVM settings in terms of GC are appropriate we should also expose this fact to users in the docs .. and then test both. In the interest of keeping it simple.. I vote for one setting...

@findepi
Copy link
Member

findepi commented Apr 23, 2024

I agree with the idea that we should use consistent JVM setting across docs, tests,

What is the minimal heap size recommended for running Trino?

@mosabua
Copy link
Member

mosabua commented Apr 23, 2024

I agree with the idea that we should use consistent JVM setting across docs, tests,

What is the minimal heap size recommended for running Trino?

I dont think we have such a recommendation as a generic advice.

@github-actions github-actions bot removed the stale label Apr 23, 2024
@wendigo wendigo closed this Apr 23, 2024
@wendigo wendigo deleted the serafin/use-g1-gc branch April 23, 2024 17:17
@findepi
Copy link
Member

findepi commented Apr 25, 2024

What is the minimal heap size recommended for running Trino?

I dont think we have such a recommendation as a generic advice.

https://trino.io/docs/current/installation/deployment.html

Large memory allocation beyond 32GB is recommended for production clusters.

@mosabua
Copy link
Member

mosabua commented Apr 25, 2024

Sure.. but that is for production clusters in typical deployments. Its not the minimal heap size that it potentially workable for a minimal deployment for simple use cases.

@findepi
Copy link
Member

findepi commented Apr 26, 2024

Agreed!
but then if we differentiate between "production clusters" and "minimal heap size that it potentially workable for a minimal deployment for simple use cases" then the only rationale given for this PR (#21311 (comment)) is no longer valid.

@mosabua
Copy link
Member

mosabua commented Apr 26, 2024

Well... even in that case we ideally use the same GC though. Anyway .. this PR is dead so no worries.

@findepi
Copy link
Member

findepi commented Apr 29, 2024

Why would we insist on using same GC algorithm if we run in widely different -- tiny -- heap for test purposes?
Does G1 make sense (benefit) if the heap is 0.5G?
Does G1 behave the same if the heap is 0.5G?
Happy to learn more.

@mosabua
Copy link
Member

mosabua commented Apr 29, 2024

In an ideal world we know what GC works best for specific workloads and machine sizes and assemble all that and expose the info to our users. I am not aware we have such knowledge anywhere or even if we tested these things. They also change across Java versions (since GC improvements are always a focus).

For now .. we only recommend one type of setting to our users in the docs and the idea of this PR is to use and hence validate that setting for all scenarios.

@wendigo
Copy link
Contributor Author

wendigo commented Apr 29, 2024

My intent was to have a gc that is close to production use cases when we land on 22 (thus G1 + region pinning). But I agree that heap sizes are too small probably for G1 to function properly, hence I'll leave this PR closed.

@martint
Copy link
Member

martint commented Apr 29, 2024

G1 can function with small heaps too. It’s the default GC in the JVM nowadays.

@wendigo
Copy link
Contributor Author

wendigo commented Apr 29, 2024

@martint do you see value in that PR?

@martint
Copy link
Member

martint commented Apr 29, 2024

Yes, I think it's a good thing to be consistent across the board with respect to what GC algorithm we use.

@wendigo wendigo restored the serafin/use-g1-gc branch April 29, 2024 19:24
@wendigo wendigo reopened this Apr 29, 2024
@findepi
Copy link
Member

findepi commented Apr 29, 2024

Does switching over to G1 increase OS memory consumption for product tests?

@wendigo
Copy link
Contributor Author

wendigo commented Apr 29, 2024

@findepi if you are concerned about local memory usage we can put some constraint on memory used by Trino docker containers in the product tests launcher. What's the ideal cap?

@wendigo wendigo merged commit a7ebb67 into master Apr 30, 2024
103 checks passed
@wendigo wendigo deleted the serafin/use-g1-gc branch April 30, 2024 09:29
@github-actions github-actions bot added this to the 446 milestone Apr 30, 2024
@findepi
Copy link
Member

findepi commented May 6, 2024

The settings that this PR removed were for that purpose.
But surely there are better settings that could be used instead, more direct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants